Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

min_relay_fee check during channel funding #1163

Merged
merged 4 commits into from
Nov 1, 2024
Merged

min_relay_fee check during channel funding #1163

merged 4 commits into from
Nov 1, 2024

Conversation

gijswijs
Copy link
Contributor

@gijswijs gijswijs commented Oct 30, 2024

This PR adds MinRelayFee to the WalletAnchor and uses it during tapchannel funding to check whether the feerate meets the minimum relay fee.

fixes: #1105

This PR depends on lightninglabs/lndclient#200 and uses a pseudoversion of that package.

go.mod should be changed before merging this PR,

@coveralls
Copy link

coveralls commented Oct 30, 2024

Pull Request Test Coverage Report for Build 11630993136

Details

  • 0 of 21 (0.0%) changed or added relevant lines in 4 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.05%) to 40.718%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chain_bridge.go 0 4 0.0%
tapgarden/mock.go 0 4 0.0%
wallet_anchor.go 0 4 0.0%
tapchannel/aux_funding_controller.go 0 9 0.0%
Files with Coverage Reduction New Missed Lines %
chain_bridge.go 1 0.0%
tapchannel/aux_leaf_signer.go 3 36.33%
universe/interface.go 4 50.22%
commitment/tap.go 4 83.91%
Totals Coverage Status
Change from base Build 11615507815: 0.05%
Covered Lines: 24620
Relevant Lines: 60464

💛 - Coveralls

@guggero
Copy link
Member

guggero commented Oct 30, 2024

Something I forgot to add in my "update to 0.18.4" PR which we should add here (since we're using another RPC field of lnd that's only available in v0.18.4-beta and later: We should bump this version to 0.18.4.

@gijswijs gijswijs force-pushed the minrelayfee branch 2 times, most recently from 768ed0c to 2e01831 Compare October 31, 2024 15:01
@gijswijs gijswijs marked this pull request as ready for review October 31, 2024 15:04
@gijswijs gijswijs requested review from guggero and jharveyb October 31, 2024 16:22
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice and clean! One request and a nit, but we're super close!

tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
To support the new `min_relay_fee_sat_per_kw` in the `lnd` RPC call
`EstimateFee`, we need to bump the `minimalCompatibleVersion` to
`0.18.4`.
With the `minimalCompatibleVersion` bumped to `0.18.4` we can assume
support for the `GetBlockHeader` RPC call. This removes the check for
support of the `GetBlockHeader` RPC call.
This commit removes the version check for lnd in the `FundVirtualPsbt`
since we can assume the new coin selection mode in the FundPsbt call
with the minimal lnd version now being `0.18.4`.
This commit adds `MinRelayFee` to the `WalletAnchor` and uses it during
tapchannel funding to check whether the feerate meets the minimum relay
fee.
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, LGTM 🎉

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to remove all these checks - LGTM!

I think we should have a follow-up PR to add similar logic to the minter, I know there have been issues there also.

I think this is the right spot, or at least related:

@gijswijs gijswijs added this pull request to the merge queue Nov 1, 2024
Merged via the queue into main with commit e6b78bd Nov 1, 2024
18 checks passed
@guggero guggero deleted the minrelayfee branch November 4, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]: Validate transaction's fee for min-relay suitability before broadcast
4 participants